Fix max_satisfaction_weight calculations for taproot.
#473
Conversation
|
This work is based on my understanding of BIP-0141 and BIP-0341. I'm not sure what is the best way to test this, any ideas will be appreciated. |
| let tree = match self.taptree() { | ||
| // key spend path: | ||
| // scriptSigLen(4) + stackLen(1) + stack[Sig]Len(1) + stack[Sig](65) | ||
| None => return Ok(4 + 1 + 1 + 65), |
There was a problem hiding this comment.
nit: the 65 here is implicitly taking into account an optional sighash flag (because we're doing max I guess this strictly correct). Maybe worth doing a 64 + 1 to indicate this.
There was a problem hiding this comment.
@LLFourn Thank you for the review.
However, at the same time, the sig+sigHash is a single stack item? Maybe a comment would be more appropriate?
|
I think you need to update some tests: for me locally. |
|
Sorry, but could you squash these commitst so the tests pass on every commit? |
* Key spend path should take into consideration the scriptSigLen, stackLen and itemLen (for the one and only stack item). * Script spend path should consider `control_block` and `script` to be items on the stack. I also restructured the function to improve readability.
evanlinjin
force-pushed
the
fix_max_satisfaction_weight_calculations
branch
from
edc583c to
8834b7f
Compare
@apoelstra All done :) |
There was a problem hiding this comment.
utACK 8834b7f. Thanks for the significant code improvement.
|
I'd like to cut a release today. Should we revert this for now? |
@apoelstra I don't think this fix conflicts with the conversation we were having in regards to what would be the definition of "max satisfaction weight". I think we should continue discussing whether we should merge #474 or go with #476 instead. Let me know your thoughts on those PRs. |
…022_10 22ac742 Merge rust-bitcoin/rust-miniscript#473: Fix `max_satisfaction_weight` calculations for taproot.
… calculations for taproot.
8834b7f992979143e0e6e27dea9a5b79d2e34f03 Fix `max_satisfaction_weight` calculations for taproot (志宇)
Pull request description:
* Key spend path should take into consideration the scriptSigLen, stackLen and itemLen (for the one and only stack item).
* Script spend path should consider `control_block` and `script` to be items on the stack.
I also restructured the function to improve readability.
ACKs for top commit:
sanket1729:
utACK 8834b7f992979143e0e6e27dea9a5b79d2e34f03. Thanks for the significant code improvement.
LLFourn:
ACK 8834b7f992979143e0e6e27dea9a5b79d2e34f03
apoelstra:
ACK 8834b7f992979143e0e6e27dea9a5b79d2e34f03
Tree-SHA512: e2f7c893bf9f9d13deae047b1d341fef513c8b1e4e0d4285684c5c420baabc89ae84033162534ee1dfd0c98e39bef01451a0e75e400cece37ec9218b2881a16f
4 participants
control_blockandscriptto be items on the stack.I also restructured the function to improve readability.